Clear gRPC Monitor request only after monitor successfully closes #1771
+21
−16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
per1234
added
topic: code
|
Actually, it seems that the problem is not the delay, but the incorrect synchronization on the gRPC server, I'm updating the PR to reflect that. |
cmaglie
force-pushed
the
small_delay_monitor_close
branch
from
3f2691b to
6625dac
Compare
cmaglie
changed the title
4 tasks
davegarthsimpson
approved these changes
Jun 17, 2022
per1234
approved these changes
Jun 17, 2022
There was a problem hiding this comment.
This change fixes the problem I reported at arduino/arduino-ide#1040 (review), where Serial Monitor still had control over the board's port when the "1200 bps touch" connection was attempted by the the upload process, causing that connection to fail with a "Serial port busy" error, and thus also causing the upload to fail due to the board never being put in bootloader mode.
Thanks Cristian!
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)What kind of change does this PR introduce?
This is a countermeasure for kernel/firwmare bugs: resources may take a few milliseconds to actually get freed for real after killing the process, or even after a regular closing.This delay makes the caller more resilient to this kind of issues.The gRPC
Monitorrequest is closed only after the monitor is successfully closed.What is the current behavior?
gRPC callers of
Monitorwill get the streaming request close as soon as the CLI asks the monitor to "close" (but without waiting for the monitor to actually close).What is the new behavior?
Now the streaming request is closed after the monitor process executes the "close" operation
Does this PR introduce a breaking change, and is titled accordingly?
No